Decouple App Hang backtrace generation from Crash Reporting - #3136
Decouple App Hang backtrace generation from Crash Reporting#3136Valpertui wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an opt-out for App Hang backtrace generation while preserving Crash Reporting.
Changes:
- Adds Swift and Objective-C Crash Reporting configuration.
- Lazily gates App Hang backtraces and introduces a distinct disabled state.
- Expands tests, documentation, API surface, and example controls.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
TestUtilities/Sources/Mocks/DatadogRUM/RUMFeatureMocks.swift |
Mocks the disabled result. |
TestUtilities/Sources/Mocks/CrashReporting/BacktraceReportingMocks.swift |
Tracks backtrace calls. |
DatadogRUM/Tests/Instrumentation/AppHangs/AppHangsWatchdogThreadTests.swift |
Tests watchdog gating. |
DatadogRUM/Tests/Instrumentation/AppHangs/AppHangsMonitorTests.swift |
Tests disabled event fields. |
DatadogRUM/Sources/RUMConfiguration.swift |
Documents the opt-out. |
DatadogRUM/Sources/Instrumentation/RUMInstrumentation.swift |
Propagates the lazy flag. |
DatadogRUM/Sources/Instrumentation/AppHangs/NonFatalAppHangsHandler.swift |
Maps disabled results. |
DatadogRUM/Sources/Instrumentation/AppHangs/AppHangsWatchdogThread.swift |
Skips backtrace generation. |
DatadogRUM/Sources/Instrumentation/AppHangs/AppHangsMonitor.swift |
Defines disabled messaging. |
DatadogRUM/Sources/Instrumentation/AppHangs/AppHang.swift |
Adds the disabled state. |
DatadogRUM/Sources/Feature/RUMFeature.swift |
Reads configuration lazily. |
DatadogRUM/RUM_FEATURE.md |
Documents feature interaction. |
DatadogInternal/Sources/BacktraceReporting/BacktraceReportingFeature.swift |
Stores the App Hang flag. |
DatadogInternal/Sources/BacktraceReporting/BacktraceReporter.swift |
Exposes registration and lookup. |
DatadogCrashReporting/Tests/CrashReportingFeatureTests.swift |
Tests feature registration. |
DatadogCrashReporting/Sources/CrashReporting.swift |
Adds public configuration APIs. |
Datadog/IntegrationUnitTests/RUM/AppHangsMonitoringTests.swift |
Tests end-to-end disabled behavior. |
Datadog/IntegrationUnitTests/CrashReporting/GeneratingBacktraceTests.swift |
Verifies other consumers remain active. |
Datadog/Example/ExampleAppDelegate.swift |
Enables example App Hang monitoring. |
Datadog/Example/Environment.swift |
Adds the launch argument. |
Datadog/Example/Debugging/DebugCrashReportingWithRUMViewController.swift |
Adds an App Hang trigger. |
Datadog/Example/Base.lproj/Main iOS.storyboard |
Adds example controls. |
CHANGELOG.md |
Records the feature. |
api-surface-swift |
Updates the public API snapshot. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86cc9ba0ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ng backtraces Until now the only way to stop generating stack traces for App Hangs was to not link `DatadogCrashReporting` at all, which also gave up crash reports. Generating the backtrace snapshots all running threads while the main thread is still blocked, so its cost adds to the duration of the hang being measured - apps with a small `appHangThreshold` may not want to pay it. `CrashReporting.Configuration.appHangBacktraceEnabled` (default `true`) lets them opt out of App Hang stack traces only. `BacktraceReportingFeature` is still registered, so the other consumers of backtrace generation - crash reports, binary images attached to error logs and RUM view events, and the public `backtraceReporter` API - are unaffected. The flag travels through `BacktraceReportingFeature` in `DatadogInternal`, since feature modules cannot import each other. RUM reads it on each hang rather than capturing it at init, so the behaviour does not depend on whether Crash Reporting was enabled before or after RUM. App Hang errors reported with backtraces disabled carry a dedicated `error.stack` message and no threads, binary images or truncation flag. The new `.disabled` case is additive to `AppHang.BacktraceGenerationResult`, so fatal hangs persisted by an earlier version still decode on the next launch. Also stops dropping a hang when the main thread ID could not be determined and backtraces are disabled - the ID is only needed to generate a backtrace.
The Example app never set `appHangThreshold`, so App Hangs were not detected at all and the new `CrashReporting.Configuration.appHangBacktraceEnabled` flag had no reachable effect. Sets a 0.5s threshold and adds an "App Hang" section to the Crash Reporting debug screen with a button that blocks the main thread for 2s. The flag is fixed at `CrashReporting.enable` time, so it cannot be a runtime toggle - it reads a `DD_DISABLE_APP_HANG_BACKTRACES` launch argument instead, matching the existing `Environment.Argument` pattern. The screen shows which state is active so the two runs can be compared.
- Restore `enable(with plugin:in:)` unchanged and add `enable(with plugin:configuration:in:)` as a separate overload with a required configuration, instead of adding a defaulted parameter to the existing one. Adding the parameter kept ordinary calls compiling but changed the exported symbol and broke unapplied references to `enable(with:in:)`. - Record `appHangBacktraceEnabled` even when a custom plugin provides no backtrace reporter. Previously the opt-out was dropped in that case and App Hangs reported the stack trace as "Crash Reporting had not been enabled". `BacktraceReportingFeature.reporter` is now optional and `register(appHangBacktraceEnabled:)` records the policy on its own; `CoreBacktraceReporter` warns and returns nil in exactly the same cases as before. - Increment `BacktraceReporterMock.generateBacktraceCallsCount` under a single write lock. `+=` through `@ReadWriteLock` took the read and write locks separately and could lose increments, which the "reporter never invoked" assertion depends on.
…mment change `make feature-docs-verify` flagged RUM_FEATURE.md as stale: `RUMConfiguration.swift` is a tracked file and this branch amends the `appHangThreshold` doc-comment, so the baseline `verified_against_commit` no longer covers the public API surface. Mirror the source doc-comment in the App hangs configuration entry and bump the frontmatter baseline. The Feature Docs Verify CI job only runs on release/hotfix branches, so this would otherwise have surfaced at release time.
…not opting out Registering the policy-only `BacktraceReportingFeature` unconditionally claimed the single registration slot, so the `get(feature:) == nil` guard in `register(backtraceReporter:)` silently dropped any reporter registered afterwards — losing `binary_images` from logs and RUM view events for apps using a custom plugin with no backtrace reporter. Record the policy only when App Hang backtraces are actually turned off, leaving the default path behaving exactly as it did before. Restore `register(backtraceReporter:)` as its own symbol and add the `appHangBacktraceEnabled` variant as an overload, so the existing compound name and mangled symbol stay unchanged for XCFramework consumers. `DatadogInternal` is not part of `DATADOG_MODULES`, so `make api-surface-verify` does not catch this class of change. Tests: the App Hang integration test picked its RUM / Crash Reporting enablement order with `oneOf`, so the lazy per-hang read of the opt-out was only exercised on about half the runs; split it into two deterministic tests. Add coverage for the reporter-less default path, for the public plugin+configuration overload forwarding its argument, and for the Objective-C configuration default and setter.
86cc9ba to
a6e2b83
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6e2b833d5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…orter is already registered `register(backtraceReporter:appHangBacktraceEnabled:)` discarded the requested policy along with the duplicate reporter: its `get(feature:) == nil` guard returned before the flag was recorded. So when a `BacktraceReportingFeature` was already registered — an integration calling the public `register(backtraceReporter:)` before enabling Crash Reporting, or a second `CrashReporting.enable` call — `CrashReporting.enable(with: .init(appHangBacktraceEnabled: false))` silently left the policy at `true` and RUM kept snapshotting all threads during App Hangs. Same class of hole as the reporter-less plugin case fixed earlier on this branch, from the opposite direction: there the opt-out had no Feature to land on, here the Feature exists but the opt-out never reached it. The first reporter still keeps the registration — that part was pre-existing and is what makes a later reporter a no-op. Only the policy is now applied on top, through `BacktraceReportingFeature.disableAppHangBacktrace()`. Opting out is deliberately one-way. `register(backtraceReporter:)` forwards `true` as a compatibility default rather than as an explicit request, so honouring it symmetrically would let a bare reporter registration silently revert an opt-out. Being one-way also makes the outcome independent of the order in which reporters are registered, matching the per-hang, lazy read the watchdog thread already does. `appHangBacktraceEnabled` therefore becomes `@ReadWriteLock private(set) var` — it is read from the App Hangs watchdog thread, once per detected hang rather than in the polling loop. No API surface change: `disableAppHangBacktrace()` is internal to `DatadogInternal` and `make api-surface-verify` is unchanged. Two tests, both confirmed failing/passing as expected before and after: - `testGivenBacktraceReporterAlreadyRegistered_whenAppHangBacktracesAreDisabled_itStillRecordsTheOptOut` reproduces the reported bug. - `testGivenAppHangBacktracesDisabled_whenRegisteringAnotherBacktraceReporter_itKeepsTheOptOut` guards the one-way direction, so a later bare registration cannot revert the opt-out.
`verified_against_commit` was `83757b8fb`, the pre-rebase SHA of what is now `9240952a0`. That object does not exist in a fresh clone, so `tools/feature-docs-verify.sh` failed on `git diff 83757b8..HEAD` rather than reporting drift: ❌ RUM_FEATURE.md: failed to diff against 83757b8. fatal: bad revision '83757b8fb..HEAD' Exactly the failure mode the update-feature-docs skill warns about in step 9 — the SHA was written before the branch was rebased, and the rebase orphaned it. The Feature Docs Verify job only runs on release/hotfix branches, so this would have surfaced at release time. Re-point it at `a4aeb05cd` and re-date the verification. No content change: this branch's only edit to a tracked file is the `appHangThreshold` doc-comment in `RUMConfiguration.swift`, already mirrored into the App hangs entry by 76dca49, and RUM's public API is untouched by the commits since. `make feature-docs-verify` now reports RUM_FEATURE.md up to date. Note that no baseline choice is rebase-proof here: the check requires a SHA at or after this branch's change to `RUMConfiguration.swift`, and every such commit is branch-local until merge. Re-run the skill if this branch is rebased or amended again before merging.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 007ff4396d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…gistered later Enabling Crash Reporting with `appHangBacktraceEnabled: false` and a custom plugin whose `backtraceReporter` is `nil` registered a `BacktraceReportingFeature` carrying only the policy. That Feature still occupied the single registration slot, so a reporter offered afterwards through the public `register(backtraceReporter:)` was dropped by the "already registered" rule and `core.backtraceReporter` stayed `nil` for the rest of the process — taking crash reports, `error.binary_images` on RUM view events and `binaryImages` on error logs with it. That directly contradicts what the option promises: it gates the App Hangs consumer only. The hazard was known — the comment at the `register(appHangBacktraceEnabled:)` call site described it as the reason the reporter-less Feature is registered *only* when opting out. This removes the hazard rather than working around it: `reporter` becomes adopt-once via `adoptReporterIfAbsent(_:)`, so the empty slot accepts the first reporter to arrive while the opt-out is retained. `BacktraceReportingFeature` is now monotonic in both of the things it carries — the reporter fills in once, the policy turns off once — so neither can be lost to registration order. That is the whole family of "single slot silently discards information" bugs closed, after the reporter-less case (9240952) and the already-registered-reporter case (a4aeb05). The narrowed reason for the `else if !configuration.appHangBacktraceEnabled` guard is now recorded at the call site: with the default there is simply no policy to record, so a reporter-less Feature would carry no information at all. Registering one is no longer harmful, just pointless. Covered by `testGivenPluginWithNoBacktraceReporter_whenAppHangBacktracesAreDisabled_itStillAcceptsALaterReporter`, confirmed failing before the fix on the `backtraceReporter` assertion and passing after. It also asserts the opt-out survives adoption. No API surface change.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96cfc96057
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
What and why?
Until now, the only way to stop generating stack traces for App Hangs was to not link
DatadogCrashReportingat all — which also gave up crash reports.CrashReporting.enable(in:)is what registersBacktraceReportingFeaturein core, andAppHangsWatchdogThreadcalls into it unconditionally, so there was no supported way to have both real crash reports and App Hang errors without stack traces.Two customer motivations:
RUM.Configuration.appHangThresholdsupports sub-second values (minimum0.1s), so this cost can be a meaningful fraction of the reported hang duration. Apps tuned to a small threshold may not want to pay it.This is an iOS-specific gap. For context on why no equivalent flag is being added elsewhere: on Android, ANR backtraces are self-contained in the RUM module (
Thread.getAllStackTraces()in the ANR detector, andApplicationExitInfofor fatal ANRs) and never depend on a separate crash-reporting module being installed, so the coupling does not exist there. Android's ANR threshold is also a fixed 5000ms, which makes the same backtrace cost negligible — whereas iOS explicitly supports sub-second thresholds. Wrappers (React Native / Flutter / KMP / Unity) callCrashReporting.enable(in:)and so keep the default; surfacing the option per-wrapper is separate, opt-in work.CHANGELOG.mdhas a[FEATURE]entry under# Unreleased.How?
The new option lives on Crash Reporting, not
RUM.Configuration, because backtrace generation is a Crash Reporting capability — RUM only consumes it.RUM.Configuration.appHangThresholdstays purely about detection.The flag travels through
DatadogInternal, sinceDatadogCrashReportingandDatadogRUMmust not import each other:Every API change is purely additive
No existing declaration gained a defaulted parameter, because that changes the compound name and the mangled symbol even though ordinary call sites keep compiling.
CrashReporting.enable(in:)andcore.register(backtraceReporter:)are kept as their own declarations, and the new forms are overloads next to them:CrashReporting.enable(in:)CrashReporting.enable(with:in:)CrashReporting.enable(with:in:)(plugin)CrashReporting.enable(with:configuration:in:)(plugin)core.register(backtraceReporter:)core.register(backtraceReporter:appHangBacktraceEnabled:)core.register(appHangBacktraceEnabled:),core.isAppHangBacktraceEnabledKeeping
enable(in:)separate is also required rather than merely preferred: a default value onconfigurationwould make the existingCrashReporting.enable()call ambiguous.Note for future changes in this area:
make api-surface-verifywould not have caught a symbol change inDatadogInternal, because that module is not inDATADOG_MODULES(Makefile:414) and so none of its public surface is tracked inapi-surface-swift. The additions above were kept additive by hand. The flip side is that they do not widen the customer-facing API surface either.Notable points
BacktraceReportingFeatureis still registered when the flag isfalse. That feature is shared by crash reports,error.binary_imageson RUM view events,binaryImageson error logs, and the publiccore.backtraceReporterAPI — so the flag cannot be implemented by skipping registration. Only the App Hangs watchdog path is gated.register(backtraceReporter:…)andregister(appHangBacktraceEnabled:)no-op when aBacktraceReportingFeatureis already registered. So the reporter-lessregister(appHangBacktraceEnabled:)is called only when a custom plugin provides no backtrace reporter and the app opted out — the one case where there is a policy worth recording. With the default, nothing extra is registered and a reporter registered later still installs, exactly as ondevelop.AppHangsMonitoris constructed, soCrashReporting.enablemay be called before or afterRUM.enable. A@Sendable () -> Boolclosure is injected into the watchdog thread (defaulting to{ true }), keeping the hot loop free of feature lookups it does not own and the unit under test injectable.DatadogCrashReportinghad not been enabled" to someone who did enable it and opted out of hang backtraces would be misleading, so.disabledcarries its ownerror.stackmessage. Never enabling Crash Reporting still yields the pre-existing message.AppHang.BacktraceGenerationResultgains a case. It isCodableand fatal hangs are persisted at hang start and replayed on next launch; existing cases keep their synthesized keys, so hangs persisted by an earlier SDK version still decode.ThreadIDit reported telemetry and dropped the hang entirely. With backtraces disabled the thread ID is not needed, so the hang is now reported instead of dropped.No default behavior changes — an app that does not touch the new option behaves exactly as before.
Resulting App Hang error fields:
error.stackthreads/binary_images/was_truncatedappHangBacktraceEnabled: falsenilnilDatadogCrashReportinghad not been enabled."nilThe last two rows are pre-existing and covered by untouched tests.
Example app. It never set
appHangThreshold, so App Hangs were not detected there at all and the flag had no reachable effect. The second commit sets a0.5sthreshold and adds an "App Hang" section to the Crash Reporting debug screen with a "Hang main thread for 2s" button. The flag is fixed atCrashReporting.enabletime so it cannot be a runtime toggle — it reads aDD_DISABLE_APP_HANG_BACKTRACESlaunch argument instead, matching the existingEnvironment.Argumentpattern, and a label shows which state is active.Feature docs.
DatadogRUM/RUM_FEATURE.mdis re-verified in its own commit:DatadogRUM/Sources/RUMConfiguration.swiftis one of itstracked_filesand this branch amends theappHangThresholddoc-comment, somake feature-docs-verifyfailed until the App Hangs entries and the frontmatter baseline were updated.How to validate
All green locally with this branch rebased onto
develop:DatadogRUM777 tests,DatadogCrashReporting64 (1 pre-existing skip),DatadogCore764,DatadogIntegrationTests211 — 0 failures; linter 0 violations in 677 files; api-surface (Swift + ObjC) up to date, delta vsdevelopis 8 insertions and 0 deletions; all 5*_FEATURE.mddocs verified.Twelve added tests, each confirmed passing by name:
AppHangsWatchdogThreadTests.testWhenBacktraceGenerationIsDisabled_itTracksAppHangWithErrorMessageAndDoesNotGenerateBacktraceAppHangsWatchdogThreadTests.testWhenBacktraceGenerationIsEnabled_itGeneratesBacktraceAppHangsMonitorTests.testWhenAppHangEndsWithBacktraceGenerationDisabled_itSendsAppHangCommandWithNoStackTrace.disabled⇒ disabled message +nilthreads / images / truncationCrashReportingFeatureTests.testByDefault_itRegistersBacktraceReporterWithAppHangBacktracesEnabledCrashReportingFeatureTests.testWhenAppHangBacktracesAreDisabled_itStillRegistersBacktraceReporterfalsestill registers the feature, with the flag offCrashReportingFeatureTests.testGivenPluginWithNoBacktraceReporter_whenAppHangBacktracesAreDisabled_itStillRecordsTheOptOutCrashReportingFeatureTests.testGivenPluginWithNoBacktraceReporter_whenAppHangBacktracesAreEnabled_itLeavesRegistrationOpenForALaterReporterCrashReportingFeatureTests.testWhenEnablingWithPluginThroughThePublicAPI_itForwardsTheConfigurationconfigurationrather than a default-constructed oneGeneratingBacktraceTests.testGivenAppHangBacktracesDisabled_whenGeneratingBacktrace_itStillGeneratesItcore.backtraceReporterunaffectedGeneratingBacktraceTests.testGivenCrashReportingNotEnabled_thenAppHangBacktracesAreNotDisabledAppHangsMonitoringTests.testGivenAppHangBacktracesDisabledInCrashReporting_whenRUMIsEnabledFirst_itTracksAppHangWithNoStackTraceAppRunner, Crash Reporting enabled after RUM — the order that proves the per-hang readAppHangsMonitoringTests.testGivenAppHangBacktracesDisabledInCrashReporting_whenCrashReportingIsEnabledFirst_itTracksAppHangWithNoStackTraceThe two enablement orders are separate tests rather than one randomized case: only the RUM-first order proves the opt-out is read per hang instead of captured when RUM is enabled, so randomizing would let that regression pass roughly half of CI runs.
DDConfiguration+apiTests.malso gained assertions on theDDCrashReporterConfigurationdefault value and setter write-through, andTestUtilities'BacktraceReporterMockgained a generation counter so "never invoked" is assertable.Manual check in the Example app: Debug Crash Reporting with RUM → "Hang main thread for 2s", then relaunch with the
DD_DISABLE_APP_HANG_BACKTRACESlaunch argument and compareerror.stackbetween the two runs.No Synthetics e2e scenario was added:
E2ETests/has no App Hangs scenario for any App Hang behavior today, its assertions live in server-side monitors rather than in this repo, and deliberately hanging the main thread sits poorly with how those scenarios are driven. TheAppHangsMonitoringTestscases above are the deepest in-repo coverage.Known, out of scope
objc_CrashReportingandobjc_CrashReportingConfigurationlive inDatadogCrashReporting/Sources/CrashReporting.swiftrather than in a+objc.swiftfile. The api-surface generator routes by filename suffix, so this module's Objective-C surface is absent fromapi-surface-objcand its members instead appear un-indented inapi-surface-swift. That predates this PR; moving the existing type is a separate change.Review checklist
DDCrashReporterConfiguration+DDCrashReporter.enableWith:make api-surfacewhen adding new APIs